Update http parser 2.9.1 v12.x by sam-github · Pull Request #30473 · nodejs/node

sam-github · 2019-11-13T18:35:32Z

We don't use nodejs/http-parser in 13.x and master, but it exists on 8, 10, and 12, and has security fixes.

I suggest we update it, I PRed all three branches:

I'm not sure if this is right way, maybe I should have just PRed 12.x, and the backports would flow down? Except 8.x doesn't get a lot of updates, its likely worth getting these known sec fixes in before it's EOL.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

sam-github · 2020-01-07T20:35:40Z

Should not be included until it can be released with a backport of #30567

sam-github · 2020-01-07T21:20:11Z

Since this introduces a breaking change in HTTP parsing I backported 02a0c74 on top of it.

See:

Impossible to make a HTTP request if response headers include control characters #30573
Cypress is unable to load site in v3.5.0 due to "Parse Error" cypress-io/cypress#5602

Original commit message below, since it now applies to both parsers (lhttp-parser aka "legacy" and llhttp), I changed the description to:

http: opt-in insecure HTTP header parsing

    http: llhttp opt-in insecure HTTP header parsing
    
    Allow insecure HTTP header parsing. Make clear it is insecure.
    
    See:
    - https://github.com/nodejs/node/pull/30553
    - https://github.com/nodejs/node/issues/27711#issuecomment-556265881
    - https://github.com/nodejs/node/issues/30515

nodejs-github-bot · 2020-01-07T21:34:23Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28271/

sam-github · 2020-01-07T22:30:21Z

#31253 should also be backported onto this, I'll do it once it has been approved.

MylesBorins · 2020-01-08T00:57:41Z

@sam-github is this ready to land now?

sam-github · 2020-01-08T01:13:11Z

It lacks code review, and also see #30473 (comment), it lacks a unit test (as does master).

12.x just went out, I think this can wait a couple days to get the above in order, it'll land in time for the next 12.x release.

Reapplying HTTP_MAX_HEADER_SIZE=8192 to http_parser.gyp. CVE-2018-12121 PR-URL: https://github.com/nodejs-private/node-private/pull/143 Ref: https://github.com/nodejs-private/security/issues/139 Ref: https://github.com/nodejs-private/http-parser-private/pull/2 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Allow insecure HTTP header parsing. Make clear it is insecure. See: - nodejs#30553 - nodejs#27711 (comment) - nodejs#30515 PR-URL: nodejs#30567 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: James M Snell <[email protected]>

Test that using --insecure-http-parser will disable validation of invalid characters in HTTP headers. See: - nodejs#30567 PR-URL: nodejs#31253 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

nodejs-github-bot · 2020-01-10T00:10:12Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28337/

sam-github · 2020-01-10T00:11:14Z

@nodejs/lts PTAL. Note that in the last commit, the backport of #31253 , I duplicated the test so that it runs with the legacy as well as the llhttp parser.

nodejs-github-bot · 2020-01-10T05:47:28Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28342/

nodejs-github-bot · 2020-01-10T14:55:04Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28348/

sam-github · 2020-01-10T15:23:48Z

This is ready to land on v12.x-staging @nodejs/lts

sam-github · 2020-01-13T20:00:53Z

Marked semver-minor, see discussion leading to #30471 (comment)

targos · 2020-01-14T07:52:57Z

Landed in c6600d1, 25f2df4, 3a7a3be, ccf9038. Thank you!

PR-URL: #30473 Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Richard Lau <[email protected]>

Reapplying HTTP_MAX_HEADER_SIZE=8192 to http_parser.gyp. CVE-2018-12121 PR-URL: nodejs-private/node-private#143 Backport-PR-URL: #30473 Ref: nodejs-private/security#139 Ref: nodejs-private/http-parser-private#2 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Allow insecure HTTP header parsing. Make clear it is insecure. See: - #30553 - #27711 (comment) - #30515 PR-URL: #30567 Backport-PR-URL: #30473 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: James M Snell <[email protected]>

Test that using --insecure-http-parser will disable validation of invalid characters in HTTP headers. See: - #30567 PR-URL: #31253 Backport-PR-URL: #30473 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Reapplying HTTP_MAX_HEADER_SIZE=8192 to http_parser.gyp. CVE-2018-12121 PR-URL: nodejs-private/node-private#143 Backport-PR-URL: #30473 Ref: nodejs-private/security#139 Ref: nodejs-private/http-parser-private#2 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Backport d41314e Original commit message: PR-URL: nodejs/node#30473 Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Richard Lau <[email protected]>

Backport 496736f Original commit message: Allow insecure HTTP header parsing. Make clear it is insecure. See: - nodejs/node#30553 - nodejs/node#27711 (comment) - nodejs/node#30515 PR-URL: nodejs/node#30567 Backport-PR-URL: nodejs/node#30473 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: James M Snell <[email protected]>

Backport ab1fcb8 Original commit message: Test that using --insecure-http-parser will disable validation of invalid characters in HTTP headers. See: - nodejs/node#30567 PR-URL: nodejs/node#31253 Backport-PR-URL: nodejs/node#30473 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Backport 496736f Original commit message: Allow insecure HTTP header parsing. Make clear it is insecure. See: - nodejs/node#30553 - nodejs/node#27711 (comment) - nodejs/node#30515 PR-URL: nodejs/node#30567 Backport-PR-URL: nodejs/node#30473 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: James M Snell <[email protected]>

Backport ab1fcb8 Original commit message: Test that using --insecure-http-parser will disable validation of invalid characters in HTTP headers. See: - nodejs/node#30567 PR-URL: nodejs/node#31253 Backport-PR-URL: nodejs/node#30473 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Backport d41314e Original commit message: PR-URL: nodejs/node#30473 Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Richard Lau <[email protected]>

Backport 496736f Original commit message: Allow insecure HTTP header parsing. Make clear it is insecure. See: - nodejs/node#30553 - nodejs/node#27711 (comment) - nodejs/node#30515 PR-URL: nodejs/node#30567 Backport-PR-URL: nodejs/node#30473 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: James M Snell <[email protected]>

Backport ab1fcb8 Original commit message: Test that using --insecure-http-parser will disable validation of invalid characters in HTTP headers. See: - nodejs/node#30567 PR-URL: nodejs/node#31253 Backport-PR-URL: nodejs/node#30473 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

nodejs-github-bot added http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. v12.x labels Nov 13, 2019

gengjiawen approved these changes Nov 15, 2019

View reviewed changes

BethGriggs force-pushed the v12.x-staging branch from d96c765 to b08601b Compare December 31, 2019 01:00

sam-github force-pushed the update-http_parser-2.9.1-v12.x branch from 2fec4f5 to 9ac0a2c Compare January 7, 2020 20:37

BridgeAR added the blocked PRs that are blocked by other issues or PRs. label Jan 7, 2020

sam-github force-pushed the update-http_parser-2.9.1-v12.x branch from ffc453a to 945711e Compare January 7, 2020 21:20

sam-github force-pushed the update-http_parser-2.9.1-v12.x branch from 945711e to f094fab Compare January 7, 2020 21:47

targos force-pushed the v12.x-staging branch 2 times, most recently from 3900f4a to 32e5c39 Compare January 8, 2020 16:34

sam-github force-pushed the update-http_parser-2.9.1-v12.x branch 2 times, most recently from 11c2aac to d694bdb Compare January 9, 2020 23:58

sam-github and others added 4 commits January 9, 2020 16:08

deps: upgrade http-parser to v2.9.1

d48a1b5

test: check that --insecure-http-parser works

2e5037b

Test that using --insecure-http-parser will disable validation of invalid characters in HTTP headers. See: - nodejs#30567 PR-URL: nodejs#31253 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

sam-github force-pushed the update-http_parser-2.9.1-v12.x branch from d694bdb to 2e5037b Compare January 10, 2020 00:09

sam-github removed the blocked PRs that are blocked by other issues or PRs. label Jan 10, 2020

sam-github mentioned this pull request Jan 10, 2020

Update http parser 2.9.1 v10.x #30471

Closed

4 tasks

jasnell approved these changes Jan 10, 2020

View reviewed changes

richardlau approved these changes Jan 10, 2020

View reviewed changes

MylesBorins force-pushed the v12.x-staging branch from 10b7951 to 0a4cfef Compare January 12, 2020 09:38

sam-github added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 13, 2020

targos closed this Jan 14, 2020

targos pushed a commit that referenced this pull request Jan 14, 2020

deps: upgrade http-parser to v2.9.1

c6600d1

PR-URL: #30473 Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Richard Lau <[email protected]>

sam-github deleted the update-http_parser-2.9.1-v12.x branch January 15, 2020 00:13

targos mentioned this pull request Jan 15, 2020

v12.15.0 release proposal #31368

Closed

Uh oh!

Conversation

sam-github commented Nov 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

sam-github commented Jan 7, 2020

Uh oh!

sam-github commented Jan 7, 2020

Uh oh!

nodejs-github-bot commented Jan 7, 2020

Uh oh!

sam-github commented Jan 7, 2020

Uh oh!

MylesBorins commented Jan 8, 2020

Uh oh!

sam-github commented Jan 8, 2020

Uh oh!

nodejs-github-bot commented Jan 10, 2020

Uh oh!

sam-github commented Jan 10, 2020

Uh oh!

nodejs-github-bot commented Jan 10, 2020

Uh oh!

nodejs-github-bot commented Jan 10, 2020

Uh oh!

sam-github commented Jan 10, 2020

Uh oh!

sam-github commented Jan 13, 2020

Uh oh!

targos commented Jan 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

sam-github commented Nov 13, 2019 •

edited

Loading

targos commented Jan 14, 2020 •

edited

Loading